An openedx_catalog app with a representation for CourseRuns [FC-0117]#479
An openedx_catalog app with a representation for CourseRuns [FC-0117]#479bradenmacdonald wants to merge 33 commits intomainfrom
Conversation
|
Thanks for the pull request, @bradenmacdonald! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
I don't know if we'll force it to be stable, but we'll probably want a |
17b313f to
8177a58
Compare
87f845c to
ccebc9d
Compare
88e2f82 to
bf0fbd6
Compare
Historical note: Apparently there was an old PR to add an opaque key for catalog courses, but it never merged: openedx/opaque-keys#87 Seems like they were called "aggregate courses" then. |
8d56dc0 to
d67189d
Compare
ac60e0e to
b83e3f7
Compare
kdmccormick
left a comment
There was a problem hiding this comment.
Partial review as I'm signing off for tonight, but overall really solid. I appreciate the comments and validation a lot. My only concerns so far are some superficial naming stuff.
| # Enforce that the course ID must end with "+run" where "run" is an exact match for the "run" field. | ||
| # This check may be removed or changed in the future if our course ID format ever changes | ||
| models.CheckConstraint( | ||
| # Note: EndsWith() on SQLite is always case-insensitive, so we code the constraint like this: | ||
| condition=Exact(Right("course_id", Length("run") + 1), Concat(models.Value("+"), "run")), | ||
| name="oex_catalog_courserun_courseid_run_match_exactly", | ||
| violation_error_message=_("The CourseRun 'run' field should match the run in the course_id key."), | ||
| ), |
There was a problem hiding this comment.
I have a sense that we'll eventually want to relax this in order to allow sites more flexibility on how they market their course runs, but I agree with adding the constraint for now and seeing how it plays out.
There was a problem hiding this comment.
Turns out that I already had to relax it a bit, because it was breaking on CCX keys like ccx-v1:org+code+run+ccx@1 which don't end with +run. In fact, I realized CCX keys break a few different assumptions here - they also violate the constraint that (org, code, run) is unique per course run, because all CCX variants of a run have the same base course ID.
There was a problem hiding this comment.
@bradenmacdonald I'm rusty on the CCX data model. Is every CCX considered a "course run" by the existing system? I know a CCX is a LearningContext, but does every CCX have a CourseOverview row?
There was a problem hiding this comment.
Yes, every CCX instance is seen as a fully separate course run by most parts of the system (except Studio, which doesn't list them nor interact with them at all; CCX runs are created/edited/managed strictly through the LMS).
I just tested this now; here are the CourseOverviews of a CCX base course plus a CCX run created from it:

4dd8ad7 to
43f7b7c
Compare
kdmccormick
left a comment
There was a problem hiding this comment.
Just a few requests for more words, looks great otherwise. I'll a look at the platform PR soon.
| # Enforce that the course ID must end with "+run" where "run" is an exact match for the "run" field. | ||
| # This check may be removed or changed in the future if our course ID format ever changes | ||
| models.CheckConstraint( | ||
| # Note: EndsWith() on SQLite is always case-insensitive, so we code the constraint like this: | ||
| condition=Exact(Right("course_id", Length("run") + 1), Concat(models.Value("+"), "run")), | ||
| name="oex_catalog_courserun_courseid_run_match_exactly", | ||
| violation_error_message=_("The CourseRun 'run' field should match the run in the course_id key."), | ||
| ), |
There was a problem hiding this comment.
@bradenmacdonald I'm rusty on the CCX data model. Is every CCX considered a "course run" by the existing system? I know a CCX is a LearningContext, but does every CCX have a CourseOverview row?
Co-authored-by: Kyle McCormick <kyle@axim.org>
kdmccormick
left a comment
There was a problem hiding this comment.
Just display_name -> title , otherwise LGTM.
I haven't manually tested this PR specifically, but I'll manually test the openedx-platform PR and call some of these API methods from the terminal, which should cover both PRs.
Co-authored-by: Kyle McCormick <kyle@axim.org>
An implementation of #469.
Related platform PR: openedx/openedx-platform#38023
Notes
edx-organizations(which also requires Pillow for thelogofield)opaque-keys, which was already an indirect dependency but not used directlydisplay_name, and catalog courses do not have names nor exist in the core platform at all. I'm proposing we add adisplay_namefield to the new coreCatalogCoursemodel to support various use cases, including the proposed new studio home page. See the code for how this can be backfilled and how runs can always override the name for each run.CatalogCourse/CourseRunobjects; just a minimal API that platform code can use to keep them in sync withCourseOverview.course_overviewsapp that syncs data from modulestore ->CourseOverview->openedx_catalog. It would be more robust and future-friendly to instead sync data directly fromSplitModulestoreCourseIndex->openedx_catalog, but it's harder to get information likedisplay_nameandlanguagein the latter case as that's not available in theSplitModulestoreCourseIndextable. It could be retrieved from Mongo though.CourseOverviewbased on thecourse_publishedsignal, any test cases in platform that want to use these models have to be sure to enable that signal, which is disabled for test by default.CourseRunwhenCourseOverviewis updated, and CourseOverview itself is updated after courses are already created, it's possible that an error will occur and theCourseRunwill never get created or updated. This will result in an error in the logs, but will not block course creation etc., so the error is likely to go unnoticed with the system as it exists today.Architecture Diagram
See ARCHITECTURE.md.
Questions
org_codeandcourse_codegood terms to use? Should I call the latternumberinstead, like other parts of the code do? Should I call the org partorg_short_name?url_codefor each CatalogCourse useful? Do we want to make it editable now, or in the future?Can be addressed later:
CourseRunaSoftDeletableModelto support course deletion without data loss? (Maybe we can't now, because soft-deleting it in that one table wouldn't affect the other tables that the system actually references. But in the future we could add this.)openedx_contentcatalog_visibility,visible_to_staff_only, andcourse_visibilitywhich all have different effects and different enum values, and also need to support "use system default")OrganizationCourse